Refresh metadata cache after failed package installation #3348
Conversation
bajertom
requested review from
psss,
lukaszachy,
happz,
thrix and
janhavlin
as code owners
bajertom
force-pushed
the
tbajer-dnf-cache-error
branch
2 times, most recently
from
c736cd9
to
793ac8a
Compare
happz
added
area | package managers
tmt/steps/prepare/install.py
Outdated
| except Exception: | ||
| # Refresh cache in case of recent but not updated change do repodata | ||
| self.guest.package_manager.refresh_metadata() | ||
| self._install() |
There was a problem hiding this comment.
I wonder whether should capture the first exception as well - this way, the original one is lost, with all its data. How about something like this?
try:
self._install()
except Exception as exc1:
# We do not have any special handling for exceptions raised by the following code.
# Wrapping them with try/except gives us a chance to attach the original exception
# to whatever the code may raise, and therefore preserve the information attached
# to the original exception.
try:
# Refresh cache in case of recent but not updated change do repodata
self.guest.package_manager.refresh_metadata()
self._install()
except Exception as exc2:
raise exc2 from ex1There was a problem hiding this comment.
You're right, that seems like an unnecessary loss of data - better to catch them both. I used your code, if that's alright:)
|
/packit test |
happz
force-pushed
the
tbajer-dnf-cache-error
branch
from
9c21ff0
to
a998cbf
Compare
|
@bajertom please, add a unit test for this new operation. It can be fairly trivial, and it would verify those commands work without crashing. See e.g. https://github.com/teemtee/tmt/blob/main/tests/unit/test_package_managers.py#L420 or other unit tests. To run new unit tests, the following command should work (with dev environment activated, and let's say you'd call the test $ hatch -e dev shell
$ pytest -vv --showlocals tests/unit/test_package_managers.py::test_refresh_metadata |
|
I've added unit tests covering the command. Since there was difference between cases in "Metadata C/cache C/created" in different distros but for the same package manager (yum) I've decided to just stick with "Metadata". Is this enough or should I catch a more specific part of the stdout in this case? Same goes for apt - it depends on whether the system is up to date or not. It's either (debian test) before if the system is up to date, so I decided to go for However I am lost with rpm-ostree packager. On my VM I can run |
bajertom
force-pushed
the
tbajer-dnf-cache-error
branch
from
ff0d1c3
to
1b833b0
Compare
LGTM. It's not perfect, which is true for other test cases in that file, we're fine. Success of the command itself is a good indication things went well.
Never seen this one before, and there are other rpm-ostree tests where I'd expect this to appear. No idea so far. Can you rebase the PR? That should make the patch easier to review and play with. |
Fixes #3277
When testing I found out that there is different behavior in dnf5 compared to dnf4, where a mere
dnf makecachewould be enough, but dnf5 needs the --refresh option as well (ordnf clean metadataordnf clean expire-cache), see https://bugzilla.redhat.com/show_bug.cgi?id=2324177Not sure about the Exception being too generic. Should I create a new one, something like InstallationError? And notify the user through logger as to why
metadata --refreshis happening?Pull Request Checklist